Verify each finding against the current code and only fix it if needed.

Inline comments:
In @.github/workflows/ci.yml:
- Around line 175-210: In the "Install Playwright" step replace the
non-deterministic install command: change the npm invocation in the step named
"Install Playwright" (currently running "npm install" in working-directory
src/e2e-playwright) to "npm ci" so dependencies are installed exactly from
package-lock.json for reproducible E2E runs; keep the subsequent "npx playwright
install --with-deps chromium" line and all other step behavior the same.

In `@CHANGELOG.md`:
- Around line 16-21: Add a new top-level changelog entry for v0.14.2 (semver and
YYYY-MM-DD) and move the "Removed dead Docker E2E infrastructure" bullet out of
the "Changed" section into a new "Removed" section, and if any security fixes
are present, add a "Security" section and move the relevant security bullet(s)
there; ensure the entry includes the standard sections: Changed, Added, Fixed,
Removed, Security (as applicable) and that the existing Angular and CI bullets
remain under "Changed".

In `@Makefile`:
- Around line 44-55: The test target test-e2e-docker currently swallows the
Playwright exit code via "|| true"; change the flow so you capture the exit code
from the npx playwright test command (store its exit status), always run the
cleanup step (docker rm -f seedsync-e2e-test), and then exit/make return using
that captured status; update the sequence around the "cd src/e2e-playwright &&
BASE_URL=... npx playwright test" invocation to save its exit code and use that
value for the final exit while keeping the container cleanup unconditional.

In `@src/angular/src/app/pages/settings/option.component.ts`:
- Around line 57-62: The check in OptionComponent that compares value ===
'********' is fragile because the sentinel is hardcoded; replace this literal
with a shared constant (e.g., REDACTED_SENTINEL) imported from a central
constants module and use that in the conditional that guards password updates
(the block referencing OptionType.Password and the value comparison in
option.component.ts); add the sentinel to a shared constants file (or bridge
from backend config) so both frontend and backend reference the same
REDACTED_SENTINEL constant and update the import/usage in the function that
calls this.type().

In `@src/e2e-playwright/.gitignore`:
- Around line 1-4: The .gitignore inside src/e2e-playwright only affects that
subtree, so add a repo-root ignore entry for /test-results/ (and optionally
/playwright-report/ and node_modules if needed) in the repository root
.gitignore and remove the generated test-results/.last-run.json from the commit
(unstage/remove it from tracking) so it is no longer tracked; locate references
to the existing src/e2e-playwright/.gitignore patterns and mirror the needed
pattern (/test-results/) at the repo root and then git rm --cached the
test-results/.last-run.json before recommitting.

In `@src/e2e-playwright/tests/dashboard.spec.ts`:
- Around line 113-124: The current test "Queue button appears for queueable
files" is vacuous because expect(typeof queueVisible).toBe("boolean") always
passes; update the test to assert meaningful behavior by either removing it or
replacing the assertion: after clicking a row via
dashboard.getFileRows().first() verify the action area is rendered (use a
selector or helper such as dashboard.getActionArea(row).isVisible()) or iterate
dashboard.getFileRows() and call dashboard.getActionButton(row, "queue") for
each row, compute queueVisible = await queueBtn.isVisible().catch(() => false)
and assert that at least one queueVisible is true; update references to queueBtn
and queueVisible accordingly so the test fails when no action area or queue
button is present.

In `@src/e2e-playwright/tests/fixtures.ts`:
- Around line 34-40: The apiGet fixture currently calls fetch without an Origin
header; update the apiGet implementation (the apiGet async fixture function) to
include the same Origin header behavior as apiFetch by passing a headers object
with "Origin": appUrl (or the appropriate origin) when calling fetch so GET
requests include an Origin for consistency with apiFetch.

In `@src/e2e-playwright/tests/logs.spec.ts`:
- Around line 63-70: The derived searchTerm can be empty, causing the subsequent
filter check (records, logs.searchInput.fill,
expect.poll(...).toBeLessThanOrEqual(count)) to be vacuous; guard against this
by detecting an empty searchTerm after deriving it from
records.first().textContent() and either (a) select a sane fallback token from
firstText (e.g., the first non-empty token) or (b) fail/skip the test with a
clear message so the filtering assertion isn't run with an empty query; update
the logic around the searchTerm variable and the call to logs.searchInput.fill
to use the non-empty fallback or short-circuit the assertion when searchTerm ===
"".

In `@src/e2e-playwright/tests/pages/path-pairs.page.ts`:
- Around line 40-52: The current form field selection in path-pairs.page.ts uses
fragile positional selectors (form.locator("input").first(), .nth(1), .nth(2)) —
update the selectors for nameInput, remoteInput, and localInput to use stable
identifiers (e.g., input[name="..."], data-testid attributes, or associated
label text via locator('label:has-text("...")').locator('.. input')) so each
branch that references fields.name, fields.remotePath, and fields.localPath
targets the correct input even if DOM order changes; change the locator
expressions for the variables nameInput, remoteInput, and localInput
accordingly.
- Around line 33-53: The fillForm method's parameter type declares unused fields
enabled and autoQueue; remove those properties from the type so the signature
becomes async fillForm(fields: { name?: string; remotePath?: string; localPath?:
string; }) in the path-pairs.page.ts file (method fillForm in that page class),
and run a quick grep for any callsites of fillForm to ensure none rely on
enabled or autoQueue—if any do, remove those args or handle them where
appropriate.

In `@src/e2e-playwright/tests/pages/sidebar.page.ts`:
- Around line 29-33: The navigateTo method uses waitForLoadState and a sidebar
selector which is unreliable for SPA navigation; replace the DOM load/state wait
and the 'a[href="/dashboard"]' selector check with a route-aware wait: after
await link.click() call await this.page.waitForURL(...) targeting the expected
route (or a pattern) so navigateTo (in sidebar.page.ts) waits for the actual URL
change rather than DOMContentLoaded or an existing sidebar link.

In `@src/e2e-playwright/tests/path-pairs.spec.ts`:
- Around line 54-72: The poll callback used inside expect.poll that calls
apiFetch("/server/pathpairs") should validate the HTTP response (res.ok) before
calling res.json(); update the callback in the block using expect.poll (and the
similar callbacks at the other poll sites) to check res.ok and either throw or
return undefined when the response is not OK so the poll keeps retrying instead
of crashing; locate the callback that awaits apiFetch and uses pairs.find
(referencing apiFetch and expect.poll) and add the res.ok check and appropriate
early return/throw to make the polling robust.

In `@src/e2e-playwright/tests/settings.spec.ts`:
- Around line 90-123: The test "select dropdown changes value and saves to
backend" declares an unused destructured parameter page in its async test
function; remove page from the parameter list (change async ({ page, apiGet })
to async ({ apiGet })) in the test declaration so only apiGet is destructured,
and verify there are no other references to page in this test (function name:
the test with description "select dropdown changes value and saves to backend",
helper: settings.getSelect).

In `@src/python/common/config.py`:
- Around line 468-478: Update the return type annotation of the static method
sensitive_property_names to use the project's typing conventions (e.g.,
typing.Dict and typing.Set) instead of built-in generics; ensure the module
imports the appropriate symbols (or uses fully-qualified typing.Dict/typing.Set)
and change the signature from def sensitive_property_names() -> dict[str,
set[str]] to use Dict[str, Set[str]] so the method and its return type follow
the codebase convention.

In `@src/python/controller/controller.py`:
- Around line 685-695: The assertions inside the critical section can raise and
skip releasing the lock (self.__model_lock), so wrap the model mutation block
that calls self.__model.add_file / remove_file / update_file and computes
diff_file in a safe construct: either replace the manual acquire/release with a
context manager (with self.__model_lock:) or use try/finally to ensure
self.__model_lock.release() always runs before any assertion or exception can
propagate (also apply the same fix around the nearby block at lines handling
assertions 727-729); keep the existing assertions but move them outside the
non-exception-safe lock or protect them inside the try/finally/with to guarantee
the lock is always released.

In `@src/python/lftp/lftp.py`:
- Line 496: The function signature for queue(...) uses a non-parameterized type
for exclude_patterns which loses element type information; update the parameter
to use a list of strings (exclude_patterns: list[str] | None) in the queue
function definition so callers and type checkers know the elements are str
(refer to the queue function and its uses around line 516 to confirm usage).

In `@src/python/tests/integration/test_web/test_handler/test_config.py`:
- Around line 95-100: Add a negative-path integration test to assert that
attempting to SET a sensitive field to the redacted sentinel is rejected: create
a new test (e.g. test_set_sensitive_field_rejects_redacted_sentinel) similar to
test_set_sensitive_field_does_not_echo_value that calls the same endpoint
("/server/config/set/lftp/remote_password/<REDACTED_SENTINEL>"), asserts a
non-200 or explicit rejection response and an error message indicating the
redacted sentinel is not allowed, and finally asserts that
self.context.config.lftp.remote_password remains unchanged; reference the
existing test_set_sensitive_field_does_not_echo_value, the GET/SET endpoint
string, and self.context.config.lftp.remote_password to locate where to add this
test.

In `@src/python/tests/integration/test_web/test_handler/test_controller.py`:
- Around line 192-240: Add the missing assertion to test_path_traversal_rejected
in TestControllerHandlerValidation: after asserting resp.status_int == 400, also
assert that self.controller.queue_command was not called (use
self.controller.queue_command.assert_not_called()) so the test mirrors the other
negative filename tests; locate the assertion to add in the
test_path_traversal_rejected method referencing self.controller.queue_command.

In `@src/python/tests/unittests/test_web/test_serialize/test_serialize_config.py`:
- Around line 91-114: Update the two assertions that expect the redaction
sentinel to use the shared constant instead of the hardcoded string: replace the
literal "********" in test_redacts_remote_password and test_redacts_api_key with
Config.REDACTED_SENTINEL so both tests assert
out_dict["lftp"]["remote_password"] == Config.REDACTED_SENTINEL and
out_dict["web"]["api_key"] == Config.REDACTED_SENTINEL; keep other assertions
unchanged and ensure SerializeConfig.config and Config.REDACTED_SENTINEL are
imported/available in the test module.

---

Outside diff comments:
In `@src/python/controller/controller.py`:
- Around line 285-297: The code currently returns a legacy default pair from
self._create_pair_context using self.__context.config.lftp even when there are
configured pairs that are all disabled; change the branching so that when
enabled_pairs is empty and pairs is non-empty you set
self.__context.status.controller.no_enabled_pairs = True and return an empty
list (no default pair), and only fall back to creating the legacy default pair
when there are zero configured pairs (i.e., pairs is empty). Update the block
that references enabled_pairs, pairs, self._create_pair_context, and
self.__context.config.lftp accordingly.

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@Makefile`:
- Around line 93-95: The help output in the Makefile is missing the
test-e2e-docker target; update the help echo block alongside the existing
test-e2e, test-e2e-headed, and test-e2e-report lines to include a brief
description for test-e2e-docker (e.g., "test-e2e-docker - Run E2E tests inside a
fresh Docker container") so users can discover this target; modify the same echo
group in the Makefile where the other test-e2e help lines are defined to add the
new entry.

In `@src/e2e-playwright/tests/dashboard.spec.ts`:
- Around line 108-110: The test's action-button selector is wrong: update the
locator used where actionButtons is assigned (the row.locator(".actions button")
call in dashboard.spec.ts) to match the actual DOM (use ".actions .button") or a
combined selector like ".actions .button, .actions button" so the actionButtons
count check (btnCount >= 1) correctly finds the div.button action controls.

In `@src/e2e-playwright/tests/pages/path-pairs.page.ts`:
- Around line 16-20: The goto() method in PathPairsPage uses
waitForLoadState("domcontentloaded") which can be flaky for Angular/SPAs;
replace or augment that with await this.page.waitForURL("/settings") (mirroring
SidebarPage.navigateTo()) to wait for SPA navigation completion, and keep the
existing waitForSelector('a[href="/dashboard"]', { timeout: 10_000 }) as a
fallback to ensure the page is ready; update the goto() implementation to use
waitForURL("/settings") instead of waitForLoadState or in addition to it.

In `@src/e2e-playwright/tests/path-pairs.spec.ts`:
- Around line 148-151: The test is using positional selectors on the 'inputs'
locator while the page object method 'fillForm' uses label-based selectors;
update the expectations to use form.getByLabel(...) (or locator.getByLabel) with
the same label texts used in 'fillForm' instead of inputs.nth(1)/nth(2) so the
test checks the values by label (e.g., assert the "Name", "Remote", "Local"
labeled inputs have the expected values) to keep selector strategy consistent
and maintainable.

In `@src/e2e-playwright/tests/settings.spec.ts`:
- Around line 35-51: The test "text field change saves to backend" mutates
persisted config (Server Address) without restoring it; capture the original
value by calling apiGet("/server/config/get") and reading
config.lftp.remote_address before changing the field, then ensure you restore
that original value in a finally block (or test.afterEach) — e.g., after the
poll completes (or on error) call the API to set the config back (use your
existing API helper similar to apiGet, e.g., apiPost or apiPut to the
appropriate config endpoint) so the UI test (settings.getTextInput("Server
Address")) leaves the backend unchanged; apply the same pattern to the other
test that changes Server Address (lines ~186-193).
- Around line 165-166: The test uses a fixed path-pair name ("temp-pair") which
causes collisions; change the literal name assignment (the object with name:
"temp-pair" used when creating the path pair in the settings test) to a
generated unique string (e.g., append a timestamp/UUID or short random suffix)
so each test run uses a distinct name; update any subsequent assertions or
lookups that reference that name to use the same generated variable rather than
the hard-coded "temp-pair".

In `@src/python/controller/controller.py`:
- Around line 296-297: Add a startup validation step that asserts required LFTP
config fields are non-None instead of using scattered "# type: ignore[arg-type]"
comments: implement a method like Controller._validate_lftp_config (or a general
Controller.validate_config) called early in Controller.__init__ or
Controller.start that checks self.__context.config.lftp.remote_address,
remote_path, local_path (and any other Optional[str] fields used later) and
raises a clear ValueError (or logs and exits) if any are missing; once this
validation exists, remove the corresponding type ignores (e.g., on uses of
self.__context.config.lftp.remote_path and local_path) so the runtime contract
is explicit and static typing is satisfied.
- Line 1261: The fallback path expression assigning dest_path in
__spawn_move_process is unreachable and unsafe (dest_path = pc.local_path if pc
else self.__pair_contexts[0].local_path); remove the fallback and require pc to
be non-None, or add an explicit validation before using self.__pair_contexts[0]
(e.g., raise a clear exception if pc is None or if __pair_contexts is empty).
Update __spawn_move_process to only use pc.local_path (or validate and raise)
and adjust callers if needed so the function contract guarantees pc is provided.

---

Outside diff comments:
In `@src/python/controller/controller.py`:
- Line 1: Run the ruff formatter on the file to fix formatting errors reported
by CI: execute `ruff format src/python/controller/controller.py` (or apply the
same formatting changes via your editor/IDE) so the file complies with ruff
style rules; then stage the modified file and re-run the tests to ensure `ruff
format --check` passes.

---

Duplicate comments:
In `@src/python/tests/integration/test_web/test_handler/test_controller.py`:
- Around line 237-241: The test_path_traversal_rejected test checks for a 400
response but misses asserting that controller.queue_command was not invoked;
update the test by adding a call to
self.controller.queue_command.assert_not_called() at the end of
test_path_traversal_rejected to mirror other negative tests (see tests at lines
with similar patterns like the ones asserting
queue_command.assert_not_called()).

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/e2e-playwright/tests/pages/path-pairs.page.ts`:
- Around line 73-75: The selector in getEnabledToggle currently picks the first
checkbox in a row which is fragile; update getEnabledToggle(pairRow: Locator) to
target the checkbox scoped to the "Enabled" label instead of using .first() —
e.g., locate the label text "Enabled" within the given pairRow and select the
associated input[type='checkbox'] so the toggle remains correct even if the
template reorders; adjust the implementation of getEnabledToggle to remove
reliance on .first() and use a label-scoped locator on the provided pairRow
Locator.

In `@src/e2e-playwright/tests/settings.spec.ts`:
- Around line 100-132: The test "select dropdown changes value and saves to
backend" mutates server state but doesn't guarantee restoration if assertions
fail; wrap the interaction and first poll assertion in a try/finally so the
original value is always restored. Specifically, after reading originalFormat
from apiGet("/server/config/get"), perform select.selectOption("json") and the
expect.poll check inside a try block, and move the restore logic
(select.selectOption(String(originalFormat)) and the subsequent expect.poll)
into the finally block to ensure cleanup even on failure; reference the test,
apiGet, settings.getSelect, select.selectOption, and expect.poll when locating
the code to change.
- Around line 63-93: The test "checkbox toggle saves to backend" toggles the
"Enable Debug" checkbox and asserts via apiGet and expect.poll but doesn't
guarantee restoration if the first poll fails; wrap the mutation and its
assertion in a try/finally inside the test so the finally always restores state:
perform the initial await checkbox.click() and the first expect.poll(...) in the
try block, and in the finally click the checkbox back and re-poll via
apiGet/expect.poll to confirm restoration; reference the test function, checkbox
(settings.getCheckbox("Enable Debug")), apiGet("/server/config/get") and
expect.poll calls when making the change.

---

Outside diff comments:
In `@src/python/controller/controller.py`:
- Around line 296-438: Many of the "# type: ignore[arg-type]" usages come from
Optional config fields being used as required in methods like
_create_pair_context (accessing
__context.config.lftp.remote_path/remote_port/remote_username,
__context.config.controller.staging_path, etc.) and _configure_lftp (reading
many __context.config.lftp properties); add a startup validation method (e.g.,
validate_config or _assert_required_config) that checks and raises a clear
exception for required lftp/controller/general/validate fields
(remote_address/remote_port/remote_username/remote_path, controller.use_staging
when staging_path is missing, net_* and pget_* constraints if required, etc.),
call it once during Controller initialization before any pair contexts are
created, and then remove the corresponding "# type: ignore[arg-type]" comments
so those accesses are statically valid and fail fast with clear error messages
when misconfigured.

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/e2e-playwright/tests/pages/path-pairs.page.ts`:
- Around line 26-30: getPairByName currently uses substring matching via hasText
and can return wrong rows (e.g., "Prod" matching "Prod-Backup"); update
getPairByName to locate ".pair-row" but require its ".pair-name" child to match
the provided name exactly using a regex anchored with ^ and $ (use a RegExp
built from the input). Also add/use an escapeRegExp helper to escape
user-supplied name before creating the RegExp so names with regex metacharacters
don't break matching.

In `@src/e2e-playwright/tests/settings.spec.ts`:
- Around line 57-59: The cleanup currently only calls apiSetConfig("lftp",
"remote_address", originalAddress) when originalAddress is truthy, which skips
restoring empty-string values and can leak config between tests; change the
teardown to always call apiSetConfig with originalAddress (remove the
surrounding if check) so apiSetConfig("lftp", "remote_address", originalAddress)
runs unconditionally; do the same for the other restoration block referenced
(the variables used there and the apiSetConfig call around lines 213-215) so
empty-string or falsy original values are correctly restored.

---

Duplicate comments:
In `@src/e2e-playwright/tests/settings.spec.ts`:
- Around line 40-44: The mutations to test state (e.g., calling await
field.clear() and await field.fill(testValue) using the local variable
testValue) are executed before the surrounding try blocks so if they throw the
finally cleanup won’t run; move those state-mutating calls (await field.clear(),
await field.fill(testValue), and the analogous mutations at the other
occurrences around lines referencing field/testValue) inside their respective
try blocks (so the try begins before any await that changes page state) and
leave only cleanup in finally to guarantee cleanup runs even on failure.
- Line 175: The generated pair name using only Date.now() in the const pairName
declaration is prone to collisions in parallel tests; update the pairName
construction (variable name: pairName) to append a compact
cryptographically-safe random suffix (e.g., use crypto.randomBytes or a short
base36 Math.random-based token) so the final string is like
`temp-pair-${Date.now()}-${randomSuffix}` and ensure the suffix uses URL-safe
characters to avoid path issues.

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/e2e-playwright/tests/settings.spec.ts`:
- Around line 192-194: The cleanup DELETE currently calls
apiFetch(`/server/pathpairs/${pair.id}`, { method: "DELETE" }) and ignores the
result; update the cleanup to await the apiFetch response, check the HTTP status
or returned success flag from the response (from the apiFetch call) and
throw/assert if deletion failed so test data doesn't leak; locate the block
using pair?.id and apiFetch in settings.spec.ts and add a failure check (e.g.,
assert status is 200/204 or response.ok) and log/throw a descriptive error if
the delete did not succeed.

---

Duplicate comments:
In `@src/e2e-playwright/tests/settings.spec.ts`:
- Around line 106-123: The selection of the "json" log format happens before the
try/finally, risking not restoring state on failures; move the call to
select.selectOption("json") into the try block (immediately before the
expect.poll call) so that the finally block that restores using
select.selectOption(String(originalFormat)) always runs, keeping the rest of the
polling logic (apiGet("/server/config/get") and expect.poll(...).toBe("json"))
unchanged.
- Line 172: The temporary pairName currently uses only Date.now() which can
collide across parallel workers; update the generation of pairName (where const
pairName = `temp-pair-${Date.now()}`) to include additional entropy such as
process.pid and a short random/UUID segment (for example
`temp-pair-${Date.now()}-${process.pid}-${Math.random().toString(36).slice(2,8)}`
or a UUID) so names are unique across workers and runs; modify the assignment in
settings.spec.ts where pairName is defined to use this stronger unique pattern.
- Around line 65-81: Move the initial mutation (await checkbox.click()) inside
the try block so the finally cleanup always runs even if the click or subsequent
polling throws; specifically, in the test around the try/finally that uses
expect.poll and apiGet (the block referencing checkbox.click(), expect.poll(...
apiGet("/server/config/get") ...).toBe(expected)), call await checkbox.click()
at the start of the try block (before the expect.poll) and keep await
checkbox.click() in the finally to toggle back.